Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Merged by Bors] - Remove task_pool parameter from par_for_each(_mut) #4705

Closed
wants to merge 11 commits into from

Conversation

james7132
Copy link
Member

@james7132 james7132 commented May 9, 2022

Objective

Fixes #3183. Requiring a &TaskPool parameter is sort of meaningless if the only correct one is to use the one provided by Res<ComputeTaskPool> all the time.

Solution

Have QueryState save a clone of the ComputeTaskPool which is used for all par_for_each functions.

Adds a small overhead of the internal Arc clone as a part of the startup, but the ergonomics win should be well worth this hardly-noticable overhead.

Updated the docs to note that it will panic the task pool is not present as a resource.

Future Work

If bevyengine/rfcs#54 is approved, we can replace these resource lookups with a static function call instead to get the ComputeTaskPool.


Changelog

Removed: The task_pool parameter of Query(State)::par_for_each(_mut). These calls will use the World's ComputeTaskPool resource instead.

Migration Guide

The task_pool parameter for Query(State)::par_for_each(_mut) has been removed. Remove these parameters from all calls to these functions.

Before:

fn parallel_system(
   task_pool: Res<ComputeTaskPool>,
   query: Query<&MyComponent>,
) {
   query.par_for_each(&task_pool, 32, |comp| {
        ...
   });
}

After:

fn parallel_system(query: Query<&MyComponent>) {
   query.par_for_each(32, |comp| {
        ...
   });
}

If using Query(State) outside of a system run by the scheduler, you may need to manually configure and initialize a ComputeTaskPool as a resource in the World.

@james7132 james7132 added A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use labels May 9, 2022
@james7132 james7132 added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label May 9, 2022
Copy link
Contributor

@IceSentry IceSentry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really nice, it was always annoying to need to add a parameter to a system if you wanted to convert it to a parallel query.

Could you add a short code snippet with a before and after in the migration guide section? It's not a big deal, but it makes writing the full migration guide much easier when examples are already taken care of.

@james7132
Copy link
Member Author

Could you add a short code snippet with a before and after in the migration guide section? It's not a big deal, but it makes writing the full migration guide much easier when examples are already taken care of.

Done!

@hymm
Copy link
Contributor

hymm commented May 10, 2022

I'm not sure if I'm missing it, but does this make it so that queries take a read lock on the ComputeTaskPool?

edit: nvm this was me missing that the task pools were cloned.

@alice-i-cecile alice-i-cecile added A-Tasks Tools for parallel and async work S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels May 10, 2022
@hymm
Copy link
Contributor

hymm commented May 10, 2022

This probably adds a small perf cost for cloning the task pool to each query even if it isn't being used. Could you run benches to see?

@james7132
Copy link
Member Author

james7132 commented May 10, 2022

The clone was removed. This only affects queries that use the par_for_each functions. Typical iteration and for_each are not impacted. The only perf impact that might be seen is during QueryState initialization, which is typically only seen during startup.

@alice-i-cecile alice-i-cecile removed the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label May 16, 2022
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
@alice-i-cecile
Copy link
Member

@james7132 can you:

  1. Fix doc nits.
  2. Rebase.
  3. Discuss the clone a bit more, ideally in a comment on that line of code.

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
@james7132 james7132 added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label May 16, 2022
@@ -61,6 +62,9 @@ impl<Q: WorldQuery, F: WorldQuery> QueryState<Q, F> {

let mut state = Self {
world_id: world.id(),
task_pool: world
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This clone is going to cause a subtle bug if anyone tries to replace the compute pool during running. The par_for_each is going ignore the pool being replaced and continue to use the original compute pool.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The more I think about this, the less such an operation makes sense. Even more reason to make TaskPools global and initialized once instead of allowing arbitrary reconfiguration mid execution.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed, but since this is one of the objections against global task pools, I'm not sure we can merge this without more clarity there.

Copy link
Member Author

@james7132 james7132 May 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking on this a bit more, this isn't too different from the status quo. You can already spawn a task, embed a clone of the TaskPool it's running on, and have it run forever. Replacing the resource would spawn a new TaskPool, but the former pool wouldn't be cleaned up.

Is this a problem? Yes. Is it one solvable within this PR? No. Does integrating this into ECS exacerbate the issue? Probably. Is it worth blocking this change on it? I don't think so. However, in turn, we should prioritize reviewing and merging #2250 or some variant of it to address this issue.

Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bors r+

bors bot pushed a commit that referenced this pull request May 30, 2022
# Objective
Fixes #3183. Requiring a `&TaskPool` parameter is sort of meaningless if the only correct one is to use the one provided by `Res<ComputeTaskPool>` all the time.

## Solution
Have `QueryState` save a clone of the `ComputeTaskPool` which is used for all `par_for_each` functions.

~~Adds a small overhead of the internal `Arc` clone as a part of the startup, but the ergonomics win should be well worth this hardly-noticable overhead.~~

Updated the docs to note that it will panic the task pool is not present as a resource.

# Future Work
If bevyengine/rfcs#54 is approved, we can replace these resource lookups with a static function call instead to get the `ComputeTaskPool`.

---

## Changelog
Removed: The `task_pool` parameter of `Query(State)::par_for_each(_mut)`. These calls will use the `World`'s `ComputeTaskPool` resource instead.

## Migration Guide
The `task_pool` parameter for `Query(State)::par_for_each(_mut)` has been removed. Remove these parameters from all calls to these functions.

Before:
```rust
fn parallel_system(
   task_pool: Res<ComputeTaskPool>,
   query: Query<&MyComponent>,
) {
   query.par_for_each(&task_pool, 32, |comp| {
        ...
   });
}
```

After:

```rust
fn parallel_system(query: Query<&MyComponent>) {
   query.par_for_each(32, |comp| {
        ...
   });
}
```

If using `Query(State)` outside of a system run by the scheduler, you may need to manually configure and initialize a `ComputeTaskPool` as a resource in the `World`.
@bors bors bot changed the title Remove task_pool parameter from par_for_each(_mut) [Merged by Bors] - Remove task_pool parameter from par_for_each(_mut) May 30, 2022
@bors bors bot closed this May 30, 2022
james7132 added a commit to james7132/bevy that referenced this pull request Jun 7, 2022
# Objective
Fixes bevyengine#3183. Requiring a `&TaskPool` parameter is sort of meaningless if the only correct one is to use the one provided by `Res<ComputeTaskPool>` all the time.

## Solution
Have `QueryState` save a clone of the `ComputeTaskPool` which is used for all `par_for_each` functions.

~~Adds a small overhead of the internal `Arc` clone as a part of the startup, but the ergonomics win should be well worth this hardly-noticable overhead.~~

Updated the docs to note that it will panic the task pool is not present as a resource.

# Future Work
If bevyengine/rfcs#54 is approved, we can replace these resource lookups with a static function call instead to get the `ComputeTaskPool`.

---

## Changelog
Removed: The `task_pool` parameter of `Query(State)::par_for_each(_mut)`. These calls will use the `World`'s `ComputeTaskPool` resource instead.

## Migration Guide
The `task_pool` parameter for `Query(State)::par_for_each(_mut)` has been removed. Remove these parameters from all calls to these functions.

Before:
```rust
fn parallel_system(
   task_pool: Res<ComputeTaskPool>,
   query: Query<&MyComponent>,
) {
   query.par_for_each(&task_pool, 32, |comp| {
        ...
   });
}
```

After:

```rust
fn parallel_system(query: Query<&MyComponent>) {
   query.par_for_each(32, |comp| {
        ...
   });
}
```

If using `Query(State)` outside of a system run by the scheduler, you may need to manually configure and initialize a `ComputeTaskPool` as a resource in the `World`.
@james7132 james7132 deleted the parallel-ergonomics branch June 22, 2022 08:23
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective
Fixes bevyengine#3183. Requiring a `&TaskPool` parameter is sort of meaningless if the only correct one is to use the one provided by `Res<ComputeTaskPool>` all the time.

## Solution
Have `QueryState` save a clone of the `ComputeTaskPool` which is used for all `par_for_each` functions.

~~Adds a small overhead of the internal `Arc` clone as a part of the startup, but the ergonomics win should be well worth this hardly-noticable overhead.~~

Updated the docs to note that it will panic the task pool is not present as a resource.

# Future Work
If bevyengine/rfcs#54 is approved, we can replace these resource lookups with a static function call instead to get the `ComputeTaskPool`.

---

## Changelog
Removed: The `task_pool` parameter of `Query(State)::par_for_each(_mut)`. These calls will use the `World`'s `ComputeTaskPool` resource instead.

## Migration Guide
The `task_pool` parameter for `Query(State)::par_for_each(_mut)` has been removed. Remove these parameters from all calls to these functions.

Before:
```rust
fn parallel_system(
   task_pool: Res<ComputeTaskPool>,
   query: Query<&MyComponent>,
) {
   query.par_for_each(&task_pool, 32, |comp| {
        ...
   });
}
```

After:

```rust
fn parallel_system(query: Query<&MyComponent>) {
   query.par_for_each(32, |comp| {
        ...
   });
}
```

If using `Query(State)` outside of a system run by the scheduler, you may need to manually configure and initialize a `ComputeTaskPool` as a resource in the `World`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events A-Tasks Tools for parallel and async work C-Usability A targeted quality-of-life change that makes Bevy easier to use M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Specify thread pool by default for the parallel iteration of queries
5 participants